-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add reactiveVars persistence options #7148
base: main
Are you sure you want to change the base?
Add reactiveVars persistence options #7148
Conversation
@PedroBern: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/ |
…ns' into feat-reactive-vars-storage-options
@benjamn I'm not sure how to fix this circleCI filesize failing, what do I have to do? If you want me to make any changes, please tell me :) |
looks like there is a value in the package.json you need to update to allow for the new file size to pass https://github.com/apollographql/apollo-client/blob/main/package.json#L56 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some minor feedback for you
Co-authored-by: Steven Rathbauer <[email protected]>
@PedroBern it looks like you have a file conflict which might be causing the incomplete checks, can you try re-basing with main and resolving the conflict to see if we can get this to pass all checks? Once they all pass hopefully @StephenBarlow can approve and merge 🤞 |
@rathpc done :) thanks for the call |
@benjamn For some reason the |
This would be amazing, our team would really benefits of having this. Let me know if I can do anything to speed this up. |
If someone is looking for a current solution, feel free to use this: import { makeVar } from '@apollo/client';
/**
* Based on this open Apollo PR: https://github.com/apollographql/apollo-client/pull/7148
*
* Example usage:
*
* const options = {
* storage: AsyncStorage,
* storageKey: '@session'
* }
*
* const [session, restoreSession] = makeVarPersisted(initialValues, options);
*
* function App () {
* const [ready, setReady] = useState(false);
*
* useEffect(() => {
* const restoreReactiveVars = async () => {
* await restoreSession();
* setReady(true);
* }
*
* restoreReactiveVars();
* }, [])
*
* if (!ready) {
* return <MyLoader />
* }
*
* return (<ApolloProvider client={client}></ApolloProvider>)
* }
*
*/
import { isString } from 'lodash';
const getCleanValueForStorage = value => {
return isString(value) ? value : JSON.stringify(value);
};
const makeVarPersisted = (initialValue, config) => {
const rv = makeVar(initialValue);
// eslint-disable-next-line func-names
const rvFn = function (newValue) {
if (arguments.length > 0) {
try {
config?.storage.setItem(
config.storageKey,
getCleanValueForStorage(newValue)
);
return rv(newValue);
} catch {
// ignore
}
} else {
return rv();
}
};
const restore = async () => {
try {
const previousValue = await config.storage.getItem(
config.storageKey
);
if (previousValue) {
rv(
isString(initialValue)
? previousValue
: JSON.parse(previousValue)
);
}
} catch {
// ignore
}
};
rvFn.onNextChange = rv.onNextChange;
return [rvFn, restore];
};
export default makeVarPersisted; Use it like so: import AsyncStorage from '@react-native-community/async-storage';
import makeVarPersisted from '../make-var-persisted';
export const initialValues = {
profile: null,
isLoggedIn: false
};
const session = makeVarPersisted(initialValues, {
storage: AsyncStorage,
storageKey: '@session'
});
export default session; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @PedroBern and others!
The spirit of this PR definitely resonates with me, and I appreciate all the work you've put into it, including the supportive feedback from @rathpc and @mikevercoelen, and the attention you paid to documentation. I apologize for keeping you waiting as long as I did.
Reactive variables clearly need some sort of persistence story, but I believe developer experience will benefit from a more comprehensive persistence system, not built specifically for reactive variables, but in which reactive variables can participate, along with various other kinds of client and cache data.
To that end, here are my initial thoughts about how we could generalize this functionality:
-
It should be possible to pass the
storage
plugin just once, perhaps as an option to theApolloClient
constructor, rather than passing it repeatedly to every reactive variable that needs to be persisted. -
Although I understand why each reactive variable needs a unique key, I'm not convinced the variables themselves need to know/manage their own keys, since there's nothing stopping two variables from claiming to have the same persistence key. Instead, I wonder if the client should be responsible for maintaining a map of uniquely keyed variables to be persisted, which would allow the variables themselves to be simple in-memory storage cells, as they are today.
-
Restoring variable data from device storage feels unnecessarily manual in your example code (the
setup
function). A single consolidated persistence system could restore everything in parallel, and provide a singlePromise
that resolves after everything has been restored (including reactive variables), making it easy to wait to render your application until all the necessary data are ready. -
If we're going to be storing cache/variable data for longer than a single browser window's lifetime, we need to have more of a plan for what happens when we change the format of the persisted data. One simple idea is to use an incrementing version, and only restore data from device storage if the version still matches. A more complicated idea would be to somehow migrate old data to the new format, instead of throwing it away. Whatever we do, I think versioning will be more manageable if it's handled at a higher level, rather than at the level of individual variables.
What do you think about these ideas? To be clear: I am not asking you to go and implement these changes, but I would love to hear your feedback before moving forward. In terms of the timeline, I would expect this functionality to ship in AC 3.4, most likely (the next minor version after Release 3.3).
Hi @benjamn! Please don't apologize, we are all busy, especially you maintaining this awesome library. Thanks for the feedback! I was concerned about 1, 2, and 3 before opening the PR, but without any planning in mind like you 😜 Passing the I totally agree with everything you have pointed out and really appreciate you asking for my feedback :) Last week I started to work on a similar API to make a persistent local relay store with local state management in mind, the approach was having my own I'm looking forward to seeing your implementations 🚀 |
@benjamn that would actually be amazing if we could address this effort further up the chain. One thing I was playing with recently was apollo-cache-persist and thought, "man this would be perfect if I could just add reactive variables to this utility on the fly" but that would require actually injecting rvs into the cache which in turn creates sort of a snowball effect of needing a local schema and field policies etc. If it is possible to store all rvs in a storage method using the same utility and keeping them adjacent to the cache without being in it, I feel like that would be the best of both worlds. Just my perspective as I just implemented using the cache persist lib. |
I found this issue after looking for ways to persist reactive vars and noticed there doesn't seem to be a ticket in https://github.com/apollographql/apollo-client/projects/3 for this. Are there still plans to add support for this in future versions of Apollo Client? |
It will be shame that such great feature would be finally lost taking into account the amount of good work done by @PedroBern |
I completely agree with the concerns raised by @benjamn last year. Would it make more sense to develop this functionality outside in a separate package like apollo-cache-persist? I know there’s similar conversation happening there too apollographql/apollo-cache-persist#361 but honestly it seems separate from cache persist since the cache is not necessarily a part of the implementation of rvs. If there was a separate library for persisting reactive variables is there an Apollo API available for iterating all defined rvs? |
I just migrated to Apollo Client 3, moving from the old local resolver setup I had, and then, like many of you, I realised that Reactive Variables can't be persisted with
Hi, @benjamn. Has there been any progress with this since your comment last November? Seeing as we're on Thanks for your time! Hope you're all safe and well. 👍 |
There are conflicts. Will somebody save us all? |
Is this ready yet? I'm trying to shift everything from Redux to reactive vars but everything is lost on page refresh. Super frustrating |
What's holding this up so far? |
How do actually query and write data using this approach I'm a bit confused here. Sorry. |
imho this implementation is better well suited. ps: for |
Variable update not working for React Native. import { makeVar } from '@apollo/client'
import AsyncStorage from '@react-native-community/async-storage'
import { is } from 'ramda'
const isString = (value) => is(value)
const getCleanValueForStorage = (value) =>
isString(value) ? value : JSON.stringify(value)
const makeVarPersisted = (initialValue, storageName) => {
const rv = makeVar(initialValue)
// eslint-disable-next-line func-names
const rvFn = function (newValue) {
if (newValue) {
try {
AsyncStorage.setItem(storageName, getCleanValueForStorage(newValue))
return rv(newValue)
} catch {
// ignore
}
} else {
return rv()
}
}
const restoreSession = async () => {
try {
const previousValue = await AsyncStorage.getItem(storageName)
if (previousValue) {
rv(isString(initialValue) ? previousValue : JSON.parse(previousValue))
}
} catch {
// ignore
}
}
rvFn.onNextChange = rv.onNextChange
return [rvFn, restoreSession]
}
export default makeVarPersisted |
This is my version of persisting reactive variables (inspired by the input above, many thanks!): function makeVarPersisted<T> (key: string, initialValue: T) {
const variable = makeVar<T>(initialValue)
async function handleOnChangeEvent (data: T) {
await AsyncStorage.setItem(key, JSON.stringify(data))
variable.onNextChange(handleOnChangeEvent)
}
async function restore () {
const previousValue = await AsyncStorage.getItem(key)
if (previousValue) {
variable(JSON.parse(previousValue))
}
}
restore()
variable.onNextChange(handleOnChangeEvent)
return variable
} |
I like this feature, but rather I'd like it would be an option on the AolloClient instance not per makeVar |
Checklist:
Summary
This PR extends the current reactive vars API to accept storage options, making it super easy to create persistent reactive variables.
New API
Restoring example